@Vurv78 & @thegrb93 I'm gonna take a break from editing this further, should be ready for a look now, when we're close to a merge I'm going to squash it

Don't squash, it'll be squashed when merged

Oh, did you manage to get an auto-squash in? If so, cool!

Oh, did you manage to get an auto-squash in? If so, cool!

Nah it's just that default wasn't set to squash and I guess sparky forgot to squash

Skimmed very quickly. Need to look into it further and actually test it later on. Just found this. Initial thoughts:

1. Shouldn't print successful tests, just print Passed/Failed. E2 test suite is currently at 100 tests (and those consist of smaller tests already). Would spam console a lot as the test suite inevitably got larger.

2. On the same note, would be nice if you could suppress that hl-zasm output print

3. I think testing at the entity level is not necessary might be a nightmare to manage. Couldn't you just have an instance manually created and track the addresses it writes to instead?

I can do 1 and 2 easily(sometime in the morning), but I'm a bit confused on what you mean by "testing at the entity level"

Skimmed very quickly. Need to look into it further and actually test it later on. Just found this. Initial thoughts:

1. Shouldn't print successful tests, just print Passed/Failed. E2 test suite is currently at 100 tests (and those consist of smaller tests already). Would spam console a lot as the test suite inevitably got larger.

2. On the same note, would be nice if you could suppress that hl-zasm output print

3. I think testing at the entity level is not necessary might be a nightmare to manage. Couldn't you just have an instance manually created and track the addresses it writes to instead?

I can do 1 and 2 easily(sometime in the morning), but I'm a bit confused on what you mean by "testing at the entity level"

Might've misread the code, I just saw the Wire_TriggerOutput in your code which likely isn't meant to be there and thought of it being entity code

Well, it was copied from the entity implementation, but it seems like I forgot to comment one out, I'll get to it in the morning

This is what the new, shortened output looks like

Now working from the dedicated server console as well as the in-game one, compilation steps were uncoupled from the CPULib compile function and now takes a fraction of the previous time due to running until compile is completed instead of limiting to 15360 compiler steps per second

@Vurv78 Does the code here meet your expectations or is there anything else you think I should work on before it's merge-worthy?

I know I'm very late to review this, but I don't like how much this test framework uses globals and relies on you creating and using a lot of functions you could easily mess up.

I would just want to keep it as simple as possible, zero globals used here:

-- Either have a comment annotation here or have it as a field in the returned table for whether the test *should* fail
local Test = {}

function Test.Setup()
    -- some initial stuff passed here, optional function
end

function Test.Teardown()
    -- also optional
end

function Test.Run(cpu, pass, fail)
    -- do asynchronous stuff and run pass() fail() function callbacks given
end

return Test

If you're going to be the only one working on it, then it's fine as long as you can work with it, but for me, this is just way too complex: I'd probably have a more fleshed out example if I had more knowledge on the cpu. This is a test suite after all and users wouldn't have to interact with it, but if it isn't easy to add tests then it might discourage future devs to create proper tests

If we want zcpu on workshop, probably want to move the tests directory to the top level, so it doesn't bog down the download.

That or include it in the ignored files in addon.json

I'm cool with just putting it in the ignored of the addon.json for now, if you know how to include it from cpu_load.lua in the autorun folder or know another way to force it to be included for those who have it when it's in the base dir let me know and I can do that instead.

I know I'm very late to review this, but I don't like how much this test framework uses globals and relies on you creating and using a lot of functions you could easily mess up.

The test framework can, and probably should go through some revision.

The initial structure this took was because we needed to wait for compile to complete since it was async(ran on a timer), since the compile is now synchronous(the one exposed by ZVMTestSuite) I could simplify it to just return the compiled code(a table of indexed numbers), so we can do away with the RunTest/RunCPU crap.

-- Either have a comment annotation here or have it as a field in the returned table for whether the test *should* fail

I'll probably make intentional fail tracked in the returned local test's table, so we can just check if it "ExpectsFail" and track those as a separate number from the passed tests when counting it on fail.

function Test.Teardown()
   -- also optional
end

If this is intended to be a "cleanup" function, I actually can't imagine what we'd need to cleanup, right now we're creating a fresh VM for every test, not passing the same one around.

I could make one that calls(if it exists) after pass or fail(just "if finished" basically) and call it something like "Test.OnFinish(bool fail)", if that's your intent.

If you're going to be the only one working on it, then it's fine as long as you can work with it, but for me, this is just way too complex: I'd probably have a more fleshed out example if I had more knowledge on the cpu. This is a test suite after all and users wouldn't have to interact with it, but if it isn't easy to add tests then it might discourage future devs to create proper tests

I'd prefer it to have the ability to be powerful without having to reinvent the wheel for most cases, but still simple enough for future devs to pick up and understand without having to look into the internals.

I'd initially thought it was intuitive but if you say otherwise I'll try and work with any suggestions you have for correcting it.

Here's an example of a potential revised test, let me know what you think of it, once we've got the "revised" test to an ideal state I can get to work on implementation and updating the tests.

-- a potential revision using example.lua as a base.

local Test = {}

function Test.Setup(CPU, TestSuite)
end

function Test.OnFinish(fail, CPU, TestSuite)
end

function Test.CompileError(msg,failCB)
end
-- ^ This one isn't used by the framework, it's a user func that's passed to
-- TestSuite:Deploy to call on a fail right now, but we could switch the func to return bool
-- on pass or fail instead to allow user to do
-- if TestSuite:Deploy() then
-- work
-- else
-- fail
-- end
-- Whichever way is deemed more intuitive, whether true on success or true on error

function Test.Run(CPU, TestSuite, passCB, failCB)
       -- Simpler function to compile and upload code to a device
       -- Compile can still exist, if the test creator needs to read or modify the results.
        TestSuite:Deploy(CPU,"x: INC R0 JMP x",Test.CompileError)
	CPU.Clk = 1
	for i = 0, 4096 do
		CPU:RunStep()
	end
	if CPU.R0 == 4096 then
                -- The CPU ran 4096 blocks that were just adding 1 to R0
                -- so it should be 4096.
		return passCB()
	else
		TestSuite.Error("R0 is not 4096! R0 is " .. tostring(CPUTest.VM.R0))
                -- Can make the failCB take similar args to Error instead.
                return failCB()
	end
end


return CPUTest

@Vurv78

function Test.Teardown()
   -- also optional
end

If this is intended to be a "cleanup" function, I actually can't imagine what we'd need to cleanup, right now we're creating a fresh VM for every test, not passing the same one around.

I could make one that calls(if it exists) after pass or fail(just "if finished" basically) and call it something like "Test.OnFinish(bool fail)", if that's your intent.

No, I just wanted the simplest api possible, if it's not necessary, great, no need to implement it

Here's an example of a potential revised test, let me know what you think of it, once we've got the "revised" test to an ideal state I can get to work on implementation and updating the tests.

-- a potential revision using example.lua as a base.

local Test = {}

function Test.Setup(CPU, TestSuite)
end

function Test.OnFinish(fail, CPU, TestSuite)
end

function Test.CompileError(msg,failCB)
end
-- ^ This one isn't used by the framework, it's a user func that's passed to
-- TestSuite:Deploy to call on a fail right now, but we could switch the func to return bool
-- on pass or fail instead to allow user to do
-- if TestSuite:Deploy() then
-- work
-- else
-- fail
-- end
-- Whichever way is deemed more intuitive, whether true on success or true on error

function Test.Run(CPU, TestSuite, passCB, failCB)
       -- Simpler function to compile and upload code to a device
       -- Compile can still exist, if the test creator needs to read or modify the results.
        TestSuite:Deploy(CPU,"x: INC R0 JMP x",Test.CompileError)
	CPU.Clk = 1
	for i = 0, 4096 do
		CPU:RunStep()
	end
	if CPU.R0 == 4096 then
                -- The CPU ran 4096 blocks that were just adding 1 to R0
                -- so it should be 4096.
		return passCB()
	else
		TestSuite.Error("R0 is not 4096! R0 is " .. tostring(CPUTest.VM.R0))
                -- Can make the failCB take similar args to Error instead.
                return failCB()
	end
end


return CPUTest

Since you said something like Teardown isn't needed, can remove that part.
For the failCB and passCB part I think I'd prefer just traditional asserts / error handling that the test suite pcalls, if you said that none of this is async anymore. If tests may still be async, maybe it's okay to leave like so. But simplifying that if else statement into just assert(CPU.R0 == 4096) would be nice.

Third option could be Test.RunAsync which has current behavior vs Test.Run which is just a simpler version without callbacks using asserts.

@Vurv78 Got them all to compile and pass with your suggestion.

With assert, we even get the file & line it occurred on, which makes it easier to track down programming errors

@Vurv78 Just wanted to follow up, was there any further modification necessary to the framework before this has your approval?

Maybe merge master back into this and update the addon.json to ignore the tests folder?